-
Notifications
You must be signed in to change notification settings - Fork 30.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: replace indexOf with includes and startsWith #13852
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work. Please feel free to also give us any feedback about the onboarding process today.
All the changes LGTM
#goodnessSquad
CI seems OK, 2 unstable results due to a flaky test. |
-1 | ||
); | ||
assert.strictEqual(stderrOutput.indexOf(internalExMessage), -1); | ||
assert(stderrOutput.includes(domainErrHandlerExMessage)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(non-blocking) Nit-Lite: Just for this case I'd use
assert.strictEqual(stderrOutput.includes(domainErrHandlerExMessage), true);
for the symmetry
@nataly87s It seems like you accidentally pushed a merge commit instead of your rebased changes. This makes it difficult to run CI against your changes and also requires a manual rebase while landing this PR. I assume you added
Please follow these steps to perform a proper rebase:
There will likely be some conflicts at this point. Running
If everything worked, you should have a single commit on top of our upstream master containing your changes. You can push the changes to your repository using
By the way, you usually don't want to create pull requests from a master branch of your own repository. Most people create a new branch for each PR, this makes it simpler to work on multiple PRs at the same time and to rebase changes. If you need further assistance, please let me know. |
@tniessen Thanks, I'll do it as soon as I get to my laptop |
@tniessen I did the merge directly in github, but i tried to do what you said anyway...
I got this message
I usually use SourceTree for anything related to git, so I'm kinda slow with it. Sorry ¯\(ツ)/¯ |
you actually need to do: git remote add upstream https://github.com/nodejs/node.git then do git remove -v should look something like: d:\code\node-cur$ git remote -v
origin https://github.com/refack/node.git (fetch)
origin https://github.com/refack/node.git (push)
upstream https://github.com/nodejs/node.git (fetch)
upstream https://github.com/nodejs/node.git (push) but with your handle |
Sorry, my bad, @refack is correct, |
Ok, I did the rebase, but then
|
@nataly87s That's what I am talking about ("force push"), you need to use |
Done. |
So git protects you from trying to push something that is not a direct extension of the current tree (something with just more commit). There are two ways to override this:
I see by now @tniessen answer, but I'm posting anyway for context and posterity... |
How's the weather there? It's been quite Israeli here (Boston) this week (33° with 90% humidity), but with crazy 100mm 2-hour rain storms every other day, and golf ball sized hail ⛳️ once a week. |
Setting it up for @tniessen to land: https://ci.nodejs.org/job/node-test-commit/10888/ 🏐 |
I don't know whats the temperature, but it's hot as hell. |
changed all the tests that the issue #12586 was relevant to
this is my first contribution #goodnessSquad
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)